Fix yaml duplicate reaction check #866

speth · 2020-06-18T00:41:54Z

Changes proposed in this pull request

Run duplicate reaction check for YAML input files (Fixes Duplicate reactions not being caught in ck2yaml #865)
Improve error messages associated with duplicate reactions and other reaction errors by showing YAML input file context, e.g.

***********************************************************************
InputFileError thrown by Kinetics::checkDuplicates:
Error on lines 1531 and 1816 of ./dup.yaml:
Undeclared duplicate reactions detected:
Reaction 325: NNH + O2 <=> HO2 + N2
Reaction 206: NNH + O2 <=> HO2 + N2

|  Line |
|  1526 | - equation: NNH + M <=> N2 + H + M  # Reaction 205
|  1527 |   type: three-body
|  1528 |   rate-constant: {A: 1.3e+14, b: -0.11, Ea: 4980.0}
|  1529 |   efficiencies: {H2: 2.0, H2O: 6.0, CH4: 2.0, CO: 1.5, CO2: 2.0, C2H6: 3.0,
|  1530 |     AR: 0.7}
>  1531 > - equation: NNH + O2 <=> HO2 + N2  # Reaction 206
            ^
|  1532 |   rate-constant: {A: 5.0e+12, b: 0.0, Ea: 0.0}
|  1533 | - equation: NNH + O <=> OH + N2  # Reaction 207
|  1534 |   rate-constant: {A: 2.5e+13, b: 0.0, Ea: 0.0}
...
|  1811 |   rate-constant: {A: 2.41e+13, b: 0.0, Ea: 0.0}
|  1812 | - equation: HO2 + C3H7 <=> O2 + C3H8  # Reaction 323
|  1813 |   rate-constant: {A: 2.55e+10, b: 0.255, Ea: -943.0}
|  1814 | - equation: HO2 + C3H7 => OH + C2H5 + CH2O  # Reaction 324
|  1815 |   rate-constant: {A: 2.41e+13, b: 0.0, Ea: 0.0}
>  1816 > - equation: NNH + O2 <=> HO2 + N2  # Reaction 206 (again)
            ^
|  1817 |   rate-constant: {A: 5.0e+12, b: 0.0, Ea: 0.0}
|  1818 | - equation: CH3 + C3H7 <=> 2 C2H5  # Reaction 325
|  1819 |   rate-constant: {A: 1.927e+13, b: -0.32, Ea: 0.0}
***********************************************************************

Replace the incomplete duplicate reaction check in ck2yaml.py with the full one done in the C++ Kinetics class, and parse the output of that error to provide line numbers of the duplicate reactions in the original input file, e.g.:

Wrote YAML mechanism file to './duplicate-reactions.yaml'.
Mechanism contains 4 species and 7 reactions.
Validating mechanism...FAILED.
Undeclared duplicate reaction C2 + H2 <=> C2H + H
found on lines 30 and 31 of  the kinetics input file.

A little bit of refactoring of AnyValue and AnyMap to make this a bit easier.

Checklist

There is a clear use-case for this code change
The commit message has a short title & references relevant issues
Build passes (scons build & scons test) and unit tests address code coverage
The pull request is ready for review

codecov · 2020-06-18T01:01:55Z

Codecov Report

Merging #866 into master will decrease coverage by 0.00%.
The diff coverage is 84.05%.

@@            Coverage Diff             @@
##           master     #866      +/-   ##
==========================================
- Coverage   70.49%   70.49%   -0.01%     
==========================================
  Files         375      375              
  Lines       45649    45671      +22     
==========================================
+ Hits        32182    32197      +15     
- Misses      13467    13474       +7

Impacted Files	Coverage Δ
src/thermo/ThermoFactory.cpp	`83.81% <ø> (ø)`
src/base/AnyMap.cpp	`83.83% <80.39%> (-0.01%)`	⬇️
src/kinetics/Kinetics.cpp	`83.85% <88.88%> (-0.36%)`	⬇️
include/cantera/base/AnyMap.h	`100.00% <100.00%> (ø)`
src/kinetics/KineticsFactory.cpp	`96.38% <100.00%> (-1.12%)`	⬇️
src/kinetics/Reaction.cpp	`87.12% <0.00%> (-0.38%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df37795...1e1340b. Read the comment docs.

bryanwweber

Thanks for fixing this @speth! Just a few small things I noticed.

interfaces/cython/cantera/ck2yaml.py

This form is useful for errors that involve two locations within the input file, such as duplicate reactions.

bryanwweber

Thanks, looks good now! I have a question below which is partly for my understanding, but may be worth adding a comment in the code for future us as well, depending on the answer.

bryanwweber · 2020-06-30T01:11:54Z

interfaces/cython/cantera/ck2yaml.py

+            if match:
+                reactions.append(int(match.group(1))-1)
+
+        if len(reactions) != 2:


Why is this check necessary? From my quick test, adding a third duplicate reaction still only shows two of them. Are there other error conditions that can reach this function?

Right, the error from Kinetics::checkDuplicates should only ever indicate two reactions. What I'm trying to hedge against is (a) the possibility there's some case where that error message doesn't have the expected format, or (b) some future change breaks this attempt at parsing the error message. If either of those happens, I'd rather output the original message from Kinetics::checkDuplicates than some error message generated caused by reactions not having the the expected number of entries.

I added some comments to show_duplicate_reactions which will hopefully make some of this clear to our future selves. 😂

Use the more comprehensive duplicate reaction check in Kinetics::checkDuplicates when validating the mechanism. Parse the error message generated by Kinetics::checkDuplicates to determine the line number in the original input file, which is more useful to users than the line number in the newly-generated YAML file.

speth force-pushed the fix-yaml-duplicate-reaction-check branch from 22f7f69 to 83f2c45 Compare June 18, 2020 18:13

bryanwweber requested changes Jun 29, 2020

View reviewed changes

interfaces/cython/cantera/ck2yaml.py Outdated Show resolved Hide resolved

interfaces/cython/cantera/ck2yaml.py Outdated Show resolved Hide resolved

interfaces/cython/cantera/ck2yaml.py Outdated Show resolved Hide resolved

interfaces/cython/cantera/ck2yaml.py Outdated Show resolved Hide resolved

speth added 4 commits June 29, 2020 19:59

[Input] Do full duplicate reaction check for YAML input files

48d0982

[Input] Move shared info from AnyMap and AnyValue to a common base class

5b76989

[Input] Add constructor for InputFileError that references two nodes

85e33d1

This form is useful for errors that involve two locations within the input file, such as duplicate reactions.

[Kinetics] Provide input file context for errors in reactions

1d51a86

speth force-pushed the fix-yaml-duplicate-reaction-check branch from 83f2c45 to 93a6e84 Compare June 30, 2020 00:00

speth requested a review from bryanwweber June 30, 2020 00:16

bryanwweber approved these changes Jun 30, 2020

View reviewed changes

speth added 2 commits June 29, 2020 21:45

[Input] Add test of mechanism validation when using ck2yaml

1e1340b

speth force-pushed the fix-yaml-duplicate-reaction-check branch from 93a6e84 to 1e1340b Compare June 30, 2020 01:48

speth merged commit 79a4163 into Cantera:master Jun 30, 2020

speth deleted the fix-yaml-duplicate-reaction-check branch July 23, 2024 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix yaml duplicate reaction check #866

Fix yaml duplicate reaction check #866

speth commented Jun 18, 2020

codecov bot commented Jun 18, 2020 •

edited

Loading

bryanwweber left a comment

bryanwweber left a comment

bryanwweber Jun 30, 2020

speth Jun 30, 2020

speth Jun 30, 2020

Fix yaml duplicate reaction check #866

Fix yaml duplicate reaction check #866

Conversation

speth commented Jun 18, 2020

codecov bot commented Jun 18, 2020 • edited Loading

Codecov Report

bryanwweber left a comment

Choose a reason for hiding this comment

bryanwweber left a comment

Choose a reason for hiding this comment

bryanwweber Jun 30, 2020

Choose a reason for hiding this comment

speth Jun 30, 2020

Choose a reason for hiding this comment

speth Jun 30, 2020

Choose a reason for hiding this comment

codecov bot commented Jun 18, 2020 •

edited

Loading